-
-
Notifications
You must be signed in to change notification settings - Fork 445
Add error handler for DroplexPackage.Drop #3900
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🥷 Code experts: jjw24 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds error handling for the DroplexPackage.Drop
method calls in plugin environment installation methods, providing better user feedback when environment installation fails.
- Wraps
DroplexPackage.Drop
calls with try-catch blocks to handle installation failures gracefully - Adds localized error messages for TypeScript and Python environment installation failures
- Implements proper error logging for debugging purposes
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
File | Description |
---|---|
Flow.Launcher/Languages/en.xaml | Adds localized error messages for TypeScript and Python environment installation failures |
Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptV2Environment.cs | Adds error handling around DroplexPackage.Drop call with user notification and logging |
Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptEnvironment.cs | Adds error handling around DroplexPackage.Drop call with user notification and logging |
Flow.Launcher.Core/ExternalPlugins/Environments/PythonEnvironment.cs | Adds error handling around DroplexPackage.Drop call with user notification and logging |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptV2Environment.cs
Show resolved
Hide resolved
Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptEnvironment.cs
Show resolved
Hide resolved
Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptV2Environment.cs
Show resolved
Hide resolved
Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptEnvironment.cs
Show resolved
Hide resolved
📝 WalkthroughWalkthroughRefactors environment installation for Python and TypeScript to run asynchronously with JTF.Run, adds try/catch error handling, and sets plugin settings only upon successful installation. Introduces localized error messages for failed installations in English resources. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PluginUI as Plugin UI
participant Env as *Environment (Python/TypeScript)*
participant Droplex as DroplexPackage
participant API as API (UI/Log)
User->>PluginUI: Trigger Install Environment
PluginUI->>Env: InstallEnvironment()
activate Env
Env->>Env: JTF.Run(async { try {...} })
Env->>Droplex: await Drop(package, installPath)
alt Success
Droplex-->>Env: Installed
Env->>Env: PluginsSettingsFilePath = ExecutablePath
else Failure
Droplex-->>Env: Exception
Env->>API: ShowMsgError(translated key)
Env->>API: LogException(className, message, exception)
end
deactivate Env
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
Flow.Launcher.Core/ExternalPlugins/Environments/PythonEnvironment.cs (1)
38-58
: Catch folder removal errors and move removal inside the guarded block.If RemoveFolderIfExists throws (locked files, ACLs), it will escape the try/catch. Move it into the same guarded JTF block so all install steps are handled uniformly and failures are logged once.
- FilesFolders.RemoveFolderIfExists(InstallPath, (s) => API.ShowMsgBox(s)); - // Python 3.11.4 is no longer Windows 7 compatible. If user is on Win 7 and // uses Python plugin they need to custom install and use v3.8.9 JTF.Run(async () => { try { + FilesFolders.RemoveFolderIfExists(InstallPath, (s) => API.ShowMsgBox(s)); await DroplexPackage.Drop(App.python_3_11_4_embeddable, InstallPath); PluginsSettingsFilePath = ExecutablePath; } catch (System.Exception e) { - API.ShowMsgError(API.GetTranslation("failToInstallPythonEnv")); - API.LogException(ClassName, "Failed to install Python environment", e); + API.ShowMsgError(API.GetTranslation("failToInstallPythonEnv")); + API.LogException(ClassName, $"Failed to install Python environment to {InstallPath}", e); } });
🧹 Nitpick comments (9)
Flow.Launcher/Languages/en.xaml (1)
38-39
: Punctuation consistency: end both sentences with a period.TypeScript string lacks a trailing period while the Python one has it. Align for consistency across resources.
- <system:String x:Key="failToInstallTypeScriptEnv">Failed to install TypeScript environment. Please try again later</system:String> + <system:String x:Key="failToInstallTypeScriptEnv">Failed to install TypeScript environment. Please try again later.</system:String>Flow.Launcher.Core/ExternalPlugins/Environments/PythonEnvironment.cs (2)
36-37
: Avoid creating a new JoinableTaskContext per instance.Creating a fresh JoinableTaskContext for each environment is unnecessary and may not align with the app’s UI thread. Prefer reusing a shared JTF (e.g., centralized in the base class or app) or at least make this static readonly to reduce overhead.
- private JoinableTaskFactory JTF { get; } = new JoinableTaskFactory(new JoinableTaskContext()); + private static readonly JoinableTaskFactory JTF = new JoinableTaskFactory(new JoinableTaskContext());If feasible, consider exposing a shared JTF from AbstractPluginEnvironment or App to ensure it’s bound to the application’s UI context.
42-44
: Win7 note: proactively short-circuit with a user-facing hint.The comment mentions Win7 incompatibility for Python 3.11.4. Consider detecting unsupported OS and skipping install with a clearer message guiding users to manually install a supported version.
I can draft a minimal OS-check guard and a localized message key if you want to include it in this PR.
Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptV2Environment.cs (3)
23-24
: Use Path.Combine for all segments to avoid mixed separators.Prefer splitting the subpath to avoid embedded backslashes and improve readability/portability.
- internal override string ExecutablePath => Path.Combine(InstallPath, "node-v16.18.0-win-x64\\node.exe"); + internal override string ExecutablePath => Path.Combine(InstallPath, "node-v16.18.0-win-x64", "node.exe");
33-34
: Consider sharing JoinableTaskFactory across environments.Same as PythonEnvironment: avoid creating a JoinableTaskContext per instance; reuse a shared JTF or make this static readonly.
- private JoinableTaskFactory JTF { get; } = new JoinableTaskFactory(new JoinableTaskContext()); + private static readonly JoinableTaskFactory JTF = new JoinableTaskFactory(new JoinableTaskContext());
39-52
: DRY: factor common installation pattern into a helper.TypeScriptEnvironment and TypeScriptV2Environment duplicate the same install pattern (remove folder, await Droplex, set settings, handle errors). Consider extracting a protected helper in the base class to reduce duplication and keep behavior consistent.
If you want, I can sketch a protected InstallWithDropAsync helper signature for AbstractPluginEnvironment that both TS environments can call.
Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptEnvironment.cs (3)
22-24
: Prefer Path.Combine with separate segments for clarity.Avoid embedding backslashes; use Path.Combine for each segment.
- internal override string ExecutablePath => Path.Combine(InstallPath, "node-v16.18.0-win-x64\\node.exe"); + internal override string ExecutablePath => Path.Combine(InstallPath, "node-v16.18.0-win-x64", "node.exe");
33-34
: JoinableTaskFactory instantiation pattern.As with the other environments, consider a shared/static JTF, or centralize it in AbstractPluginEnvironment/App to align with the app’s UI context and reduce allocations.
- private JoinableTaskFactory JTF { get; } = new JoinableTaskFactory(new JoinableTaskContext()); + private static readonly JoinableTaskFactory JTF = new JoinableTaskFactory(new JoinableTaskContext());
39-52
: Include InstallPath in error log for easier diagnostics.Adding the target path to the log message speeds up troubleshooting.
- API.LogException(ClassName, "Failed to install TypeScript environment", e); + API.LogException(ClassName, $"Failed to install TypeScript environment to {InstallPath}", e);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Flow.Launcher.Core/ExternalPlugins/Environments/PythonEnvironment.cs
(2 hunks)Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptEnvironment.cs
(2 hunks)Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptV2Environment.cs
(2 hunks)Flow.Launcher/Languages/en.xaml
(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/Environments/PythonEnvironment.cs
📚 Learning: 2025-07-21T09:19:49.684Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/Environments/PythonEnvironment.cs
🪛 GitHub Check: Check Spelling
Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptEnvironment.cs
[warning] 39-39:
JTF
is not a recognized word. (unrecognized-spelling)
Flow.Launcher.Core/ExternalPlugins/Environments/PythonEnvironment.cs
[warning] 44-44:
JTF
is not a recognized word. (unrecognized-spelling)
[warning] 44-44:
JTF
is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (3)
Flow.Launcher.Core/ExternalPlugins/Environments/PythonEnvironment.cs (1)
44-57
: Solid: exceptions handled and settings updated only on success.Catching exceptions from DroplexPackage.Drop, surfacing a localized error, and only setting PluginsSettingsFilePath after a successful await is the right approach. Logging includes class context. Looks good.
Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptV2Environment.cs (1)
39-52
: Good: installation is awaited, errors localized, and settings set on success.The guarded JTF.Run block correctly awaits Droplex, shows a localized error on failure, and assigns PluginsSettingsFilePath only after success. Logging includes class context.
Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptEnvironment.cs (1)
39-52
: Well-scoped error handling and post-success settings update.Matches the intended behavior: await install, localize error, and only set PluginsSettingsFilePath on success. Logging includes context. Good change.
Add error handler for DroplexPackage.Drop
Resolve #3856